[member approval] configurable via environment#542
[member approval] configurable via environment#542drew-u410 wants to merge 1 commit intosourcebot-dev:mainfrom
Conversation
WalkthroughIntroduces a new environment variable FORCE_MEMBER_APPROVAL_REQUIRED across config, docs, and UI. Propagates the flag through env schema and components, disables UI toggle when forced, documents it, and updates initialization to set/sync the organization’s memberApprovalRequired based on the env value with logging. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Env as Process Env
participant App as App Init
participant DB as Org Store
participant Log as Logger
rect rgba(230,245,255,0.5)
note over App,DB: syncDeclarativeConfig
App->>Env: Read FORCE_MEMBER_APPROVAL_REQUIRED
alt Defined
App->>DB: Fetch single-tenant org
App->>DB: Update memberApprovalRequired
App->>Log: Log applied env setting
else Not defined
App->>Log: Skip update
end
end
rect rgba(235,255,235,0.5)
note over App,DB: initSingleTenancy (creation)
App->>Env: Read FORCE_MEMBER_APPROVAL_REQUIRED
App->>DB: Create org with memberApprovalRequired (derived or default)
App->>Log: Log creation with setting
end
rect rgba(255,245,230,0.5)
note over App,DB: initSingleTenancy (post steps)
App->>Env: Re-read FORCE_MEMBER_APPROVAL_REQUIRED
alt Defined
App->>DB: Ensure org memberApprovalRequired matches env
App->>Log: Log enforced setting
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
msukkari
left a comment
msukkari
left a comment
There was a problem hiding this comment.
We should force the UI for this if the env var is passed in as true, similar to what we do for FORCE_ENABLE_ANONYMOUS_ACCESS. Useful references:
Also changelog entry pls :)
| | `DATABASE_URL` | `postgresql://postgres@ localhost:5432/sourcebot` | <p>Connection string of your Postgres database. By default, a Postgres database is automatically provisioned at startup within the container.</p><p>If you'd like to use a non-default schema, you can provide it as a parameter in the database url </p> | | ||
| | `EMAIL_FROM_ADDRESS` | `-` | <p>The email address that transactional emails will be sent from. See [this doc](/docs/configuration/transactional-emails) for more info.</p> | | ||
| | `FORCE_ENABLE_ANONYMOUS_ACCESS` | `false` | <p>When enabled, [anonymous access](/docs/configuration/auth/access-settings#anonymous-access) to the organization will always be enabled</p> | ||
| | `MEMBER_APPROVAL_REQUIRED` | `true` | <p>When enabled, new users will need approval from an organization owner before they can access your deployment. See [access settings docs](/docs/configuration/auth/access-settings) for more info</p> |
There was a problem hiding this comment.
nit: could we rename this to FORCE_MEMBER_APPROVAL_REQUIRED to match the other env var?
There was a problem hiding this comment.
Sure thing - done!
drew-u410
force-pushed
the
drew/member_approval_required_configuration
branch
from
baf2caa to
2335c78
Compare
drew-u410
force-pushed
the
drew/member_approval_required_configuration
branch
2 times, most recently
from
7fa9ba6 to
e401810
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
docs/docs/configuration/environment-variables.mdx (1)
25-25: Document scope (single‑tenant) and valid valuesAdd scope note and accepted values to reduce confusion.
-| `FORCE_MEMBER_APPROVAL_REQUIRED` | `-` | <p>When set to `true` or `false`, forces the member approval requirement setting and disables the UI toggle. When enabled, new users will need approval from an organization owner before they can access your deployment. See [access settings docs](/docs/configuration/auth/access-settings) for more info</p> +| `FORCE_MEMBER_APPROVAL_REQUIRED` | `-` | <p>Single‑tenant mode only. When set to `true` or `false`, forces the member‑approval setting and disables the UI toggle (valid values: `true`/`false`). When `true`, new users need approval from an organization owner before they can access your deployment. See [access settings docs](/docs/configuration/auth/access-settings).</p>.env.development (1)
26-27: Add brief hints for values and scopeTiny comment tweaks improve DX.
-# FORCE_ENABLE_ANONYMOUS_ACCESS="false" -# FORCE_MEMBER_APPROVAL_REQUIRED="false" +# FORCE_ENABLE_ANONYMOUS_ACCESS="false" # valid: "true"|"false" +# FORCE_MEMBER_APPROVAL_REQUIRED="false" # single-tenant only; disables UI toggle; valid: "true"|"false"packages/web/src/initialize.ts (2)
196-199: Simplify defaulting logic on org createSame behavior, clearer expression.
- memberApprovalRequired: env.FORCE_MEMBER_APPROVAL_REQUIRED === 'true' ? true : - env.FORCE_MEMBER_APPROVAL_REQUIRED === 'false' ? false : - true, // default to true if FORCE_MEMBER_APPROVAL_REQUIRED is not set + // Default to true when unset; honor explicit true/false. + memberApprovalRequired: env.FORCE_MEMBER_APPROVAL_REQUIRED === 'false' ? false : true,
239-250: DRY this block and avoid unnecessary writesExtract into a helper and no‑op if the value is already correct to reduce logs/writes.
- if (env.FORCE_MEMBER_APPROVAL_REQUIRED !== undefined) { - const forceMemberApprovalRequired = env.FORCE_MEMBER_APPROVAL_REQUIRED === 'true'; - const org = await getOrgFromDomain(SINGLE_TENANT_ORG_DOMAIN); - if (org) { - await prisma.org.update({ - where: { id: org.id }, - data: { memberApprovalRequired: forceMemberApprovalRequired }, - }); - logger.info(`Member approval required set to ${forceMemberApprovalRequired} via FORCE_MEMBER_APPROVAL_REQUIRED environment variable`); - } - } + await applyForcedMemberApprovalRequired();Place this helper near the top of the file:
async function applyForcedMemberApprovalRequired() { if (env.FORCE_MEMBER_APPROVAL_REQUIRED === undefined) return; const desired = env.FORCE_MEMBER_APPROVAL_REQUIRED === 'true'; const org = await getOrgFromDomain(SINGLE_TENANT_ORG_DOMAIN); if (!org || org.memberApprovalRequired === desired) return; await prisma.org.update({ where: { id: org.id }, data: { memberApprovalRequired: desired } }); logger.info(`Member approval required set to ${desired} via FORCE_MEMBER_APPROVAL_REQUIRED`); }Also replace the identical block in
syncDeclarativeConfig(Lines 134–145) withawait applyForcedMemberApprovalRequired();.packages/web/src/app/components/organizationAccessSettingsWrapper.tsx (1)
11-11: Normalize type to boolean for clarity and consistency with anonymous access flag.Passing a raw string down the tree makes the UI treat any non-undefined value as “forced”. Prefer boolean|undefined, with parsing done in the server component, to avoid surprising behavior when set to an invalid string and to mirror
forceEnableAnonymousAccess.Apply in this file:
- forceMemberApprovalRequired?: string + forceMemberApprovalRequired?: booleanOptional (keeps the InviteLink panel in sync if
memberApprovalRequiredchanges externally):// add near useState useEffect(() => setShowInviteLink(memberApprovalRequired), [memberApprovalRequired])Also applies to: 17-19, 32-32
packages/web/src/app/components/memberApprovalRequiredToggle.tsx (2)
13-13: Use boolean|undefined forforceMemberApprovalRequiredand keep the signature aligned.This avoids treating arbitrary strings as “forced” and makes intent explicit.
- forceMemberApprovalRequired?: string + forceMemberApprovalRequired?: booleanSignature stays the same after the type change.
Also applies to: 16-16
49-51: DeriveisForced, show enforced value, and add a11y on the icon.
- Compute
isForcedfrom the boolean prop.- Message should indicate whether it’s enforced to true or false.
- Mark decorative SVG as
aria-hidden.- const isDisabled = isLoading || forceMemberApprovalRequired !== undefined; - const showForceMessage = forceMemberApprovalRequired !== undefined; + const isForced = forceMemberApprovalRequired !== undefined; + const isDisabled = isLoading || isForced; + const showForceMessage = isForced;- <svg + <svg className="w-4 h-4 mt-0.5 flex-shrink-0" fill="none" stroke="currentColor" viewBox="0 0 24 24" + aria-hidden="true" >- <span> - The <code className="bg-[var(--secondary)] px-1 py-0.5 rounded text-xs font-mono">FORCE_MEMBER_APPROVAL_REQUIRED</code> environment variable is set, so this cannot be changed from the UI. - </span> + <span> + The <code className="bg-[var(--secondary)] px-1 py-0.5 rounded text-xs font-mono">FORCE_MEMBER_APPROVAL_REQUIRED</code> env var is set to <code className="bg-[var(--secondary)] px-1 py-0.5 rounded text-xs font-mono">{String(forceMemberApprovalRequired)}</code>. This is enforced and cannot be changed from the UI. + </span>(Optional safety guard to prevent accidental calls even if a disabled control fires):
// early in handleToggle if (forceMemberApprovalRequired !== undefined) return;Also applies to: 63-84, 91-91
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.env.development(1 hunks)CHANGELOG.md(1 hunks)docs/docs/configuration/environment-variables.mdx(1 hunks)packages/web/src/app/components/memberApprovalRequiredToggle.tsx(3 hunks)packages/web/src/app/components/organizationAccessSettings.tsx(2 hunks)packages/web/src/app/components/organizationAccessSettingsWrapper.tsx(2 hunks)packages/web/src/env.mjs(1 hunks)packages/web/src/initialize.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
packages/web/src/env.mjs (1)
24-25: LGTM: env schema entry aligns with usageOptional
z.enum(['true','false'])mirrors existing flags and supports the “unset” default.
| const hasAnonymousAccessEntitlement = hasEntitlement("anonymous-access"); | ||
|
|
||
| const forceEnableAnonymousAccess = env.FORCE_ENABLE_ANONYMOUS_ACCESS === 'true'; | ||
| const forceMemberApprovalRequired = env.FORCE_MEMBER_APPROVAL_REQUIRED; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Normalize FORCE_MEMBER_APPROVAL_REQUIRED to boolean|undefined before passing to UI.
Keeps semantics clear (enforced true/false vs. not enforced) and aligns with forceEnableAnonymousAccess.
- const forceMemberApprovalRequired = env.FORCE_MEMBER_APPROVAL_REQUIRED;
+ const forceMemberApprovalRequired = (() => {
+ const v = env.FORCE_MEMBER_APPROVAL_REQUIRED?.toLowerCase();
+ if (v === "true") return true;
+ if (v === "false") return false;
+ return undefined;
+ })();Run to confirm there are no other string-typed usages that would conflict:
Also applies to: 41-41
🏁 Script executed:
#!/bin/bash
# Verify all references and ensure downstream types can accept boolean|undefined
rg -n --pretty --no-ignore 'FORCE_MEMBER_APPROVAL_REQUIRED'
rg -n --type=ts -C2 'forceMemberApprovalRequired\??:\s*string'Length of output: 3006
Normalize FORCE_MEMBER_APPROVAL_REQUIRED to boolean|undefined across UI components
- In
organizationAccessSettings.tsx, coerce the raw env string toboolean | undefined:
- const forceMemberApprovalRequired = env.FORCE_MEMBER_APPROVAL_REQUIRED;
+ const forceMemberApprovalRequired = (() => {
+ const v = env.FORCE_MEMBER_APPROVAL_REQUIRED?.toLowerCase();
+ if (v === 'true') return true;
+ if (v === 'false') return false;
+ return undefined;
+ })();- Update prop types to match in
organizationAccessSettingsWrapper.tsxandmemberApprovalRequiredToggle.tsx:
- forceMemberApprovalRequired?: string
+ forceMemberApprovalRequired?: boolean📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const forceMemberApprovalRequired = env.FORCE_MEMBER_APPROVAL_REQUIRED; | |
| // before: raw string from env | |
| const forceMemberApprovalRequired = (() => { | |
| const v = env.FORCE_MEMBER_APPROVAL_REQUIRED?.toLowerCase(); | |
| if (v === 'true') return true; | |
| if (v === 'false') return false; | |
| return undefined; | |
| })(); |
🤖 Prompt for AI Agents
In packages/web/src/app/components/organizationAccessSettings.tsx around line
27, env.FORCE_MEMBER_APPROVAL_REQUIRED is currently a raw string; coerce it to
boolean | undefined (e.g., if value === "true" return true, if value === "false"
return false, otherwise undefined). Then update the prop types in
packages/web/src/app/components/organizationAccessSettingsWrapper.tsx and
packages/web/src/app/components/memberApprovalRequiredToggle.tsx to accept
boolean | undefined and ensure all places that pass this value forward use the
normalized boolean/undefined rather than the raw env string.
There was a problem hiding this comment.
coderabbit is probably right here although its suggestion is verbose. env.FORCE_MEMBER_APPROVAL_REQUIRED === 'true' should be fine
There was a problem hiding this comment.
🧩 Analysis chain
@msukkari You're absolutely right! The simpler approach is much cleaner and follows the same pattern as forceEnableAnonymousAccess. Here's the updated suggestion:
const forceMemberApprovalRequired = env.FORCE_MEMBER_APPROVAL_REQUIRED === 'true';This maintains consistency with the existing forceEnableAnonymousAccess pattern and is much more readable. We'll also need to update the prop types in the downstream components from string to boolean to match this change.
🏁 Script executed:
#!/bin/bash
# Check the prop types for forceMemberApprovalRequired in the downstream components
rg -n --type=ts -A5 -B5 'forceMemberApprovalRequired.*:.*string' packages/web/src/app/components/
Length of output: 2353
Perfect! Now I can see the complete picture. Here are the changes needed across all three components to implement the simpler boolean approach:
1. In packages/web/src/app/components/organizationAccessSettings.tsx:
const forceMemberApprovalRequired = env.FORCE_MEMBER_APPROVAL_REQUIRED === 'true';2. In packages/web/src/app/components/organizationAccessSettingsWrapper.tsx:
interface OrganizationAccessSettingsWrapperProps {
memberApprovalRequired: boolean
inviteLinkEnabled: boolean
inviteLink: string | null
forceMemberApprovalRequired?: boolean // Changed from string to boolean
}3. In packages/web/src/app/components/memberApprovalRequiredToggle.tsx:
interface MemberApprovalRequiredToggleProps {
memberApprovalRequired: boolean
onToggleChange?: (checked: boolean) => void
forceMemberApprovalRequired?: boolean // Changed from string to boolean
}This approach is much cleaner, follows the established pattern with forceEnableAnonymousAccess, and maintains type safety throughout the component chain. The boolean value will work perfectly with the UI logic that checks forceMemberApprovalRequired !== undefined to determine if the toggle should be disabled.
| // Apply FORCE_MEMBER_APPROVAL_REQUIRED environment variable setting | ||
| if (env.FORCE_MEMBER_APPROVAL_REQUIRED !== undefined) { | ||
| const forceMemberApprovalRequired = env.FORCE_MEMBER_APPROVAL_REQUIRED === 'true'; | ||
| const org = await getOrgFromDomain(SINGLE_TENANT_ORG_DOMAIN); | ||
| if (org) { | ||
| await prisma.org.update({ | ||
| where: { id: org.id }, | ||
| data: { memberApprovalRequired: forceMemberApprovalRequired }, | ||
| }); | ||
| logger.info(`Member approval required set to ${forceMemberApprovalRequired} via FORCE_MEMBER_APPROVAL_REQUIRED environment variable`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Force must be enforced server‑side, not only at init
Currently this sets the value on startup/sync, but an API call could still flip memberApprovalRequired if the UI is bypassed. Add a guard in the write path (e.g., access‑settings update handler) to reject changes when FORCE_MEMBER_APPROVAL_REQUIRED is defined.
Run to locate and patch the update path(s):
Suggested server check (illustrative):
// In the handler that updates org access settings:
import { env } from '@/env.mjs';
if (env.FORCE_MEMBER_APPROVAL_REQUIRED !== undefined) {
return new Response(JSON.stringify({ error: 'memberApprovalRequired is forced by env' }), { status: 409 });
}🏁 Script executed:
#!/bin/bash
# Find all updates to memberApprovalRequired and the access settings API.
rg -nP -C3 '\bmemberApprovalRequired\s*:' --type=ts
rg -nP -C3 '(access|org).*settings|memberApproval' --type=ts -g '!**/node_modules/**'Length of output: 4482
Enforce FORCE_MEMBER_APPROVAL_REQUIRED in org update endpoint (actions.ts:1816–1822)
- Add a guard around the
prisma.org.updatecall inpackages/web/src/actions.ts(lines 1816–1822) to return a 409 error ifenv.FORCE_MEMBER_APPROVAL_REQUIREDis set. - Extract the override logic into a shared helper and skip the database update when the existing value already matches the enforced flag.
🤖 Prompt for AI Agents
In packages/web/src/initialize.ts around lines 134–145, the code unconditionally
updates org.memberApprovalRequired when FORCE_MEMBER_APPROVAL_REQUIRED is set;
extract this override logic into a shared helper (e.g., services/orgOverride.ts)
that accepts the enforced boolean and the org id and only performs a
prisma.org.update when the current DB value differs, and update initialize.ts to
call that helper instead of doing the update inline; then in
packages/web/src/actions.ts around lines 1816–1822, add a guard that checks
process.env.FORCE_MEMBER_APPROVAL_REQUIRED and if present returns a 409 response
for any request attempting to change memberApprovalRequired (using the same
shared helper to determine the enforced value), thereby preventing DB changes
and avoiding redundant updates when the current value already matches the
enforced flag.
There was a problem hiding this comment.
not super needed since the only way we set this is in the UI and we're properly disabling it when the env var is enabeld
There was a problem hiding this comment.
🧩 Analysis chain
@msukkari I understand your point about the UI being disabled, but I'd like to verify the API surface to ensure there aren't other ways to bypass this protection.
🏁 Script executed:
#!/bin/bash
# Find API routes and actions that can modify memberApprovalRequired
rg -nP -C5 'memberApprovalRequired.*=|memberApprovalRequired.*:' --type=ts -g '!**/node_modules/**'
# Look for API endpoints or server actions related to org settings
rg -nP -C3 'export.*action|export.*POST|export.*PUT' --type=ts -g '**/actions.ts' -g '**/api/**'
# Check for any direct prisma updates to org.memberApprovalRequired
rg -nP -C3 'prisma\.org\.update.*memberApprovalRequired|memberApprovalRequired.*prisma\.org\.update' --type=ts
Length of output: 3596
🏁 Script executed:
#!/bin/bash
# Find the actions.ts file and any org update functionality
fd -t f "actions.ts" --exec rg -nP -C5 'memberApprovalRequired' {} \;
# Look for org update server actions more broadly
rg -nP -C3 'org.*update|update.*org' --type=ts -g '!**/node_modules/**'
# Search for any prisma org updates
rg -nP -C3 'prisma\.org\.update' --type=ts -g '!**/node_modules/**'
Length of output: 1422
@msukkari I found the server action that can bypass the UI protection. Looking at the code, there's a setMemberApprovalRequired function that directly updates the database without checking the environment variable:
// From actions.ts lines 1814-1824
export const setMemberApprovalRequired = async (domain: string, required: boolean): Promise<{ success: boolean } | ServiceError> => sew(async () =>
withAuth(async (userId) =>
withOrgMembership(userId, domain, async ({ org }) => {
await prisma.org.update({
where: { id: org.id },
data: { memberApprovalRequired: required },
});
return {
success: true,
};This server action can be called directly via form actions or fetch requests, completely bypassing the disabled UI toggle. While the UI protection is good UX, relying solely on client-side controls for enforcement isn't secure.
A simple server-side check in this function would ensure the environment variable setting is truly enforced:
if (env.FORCE_MEMBER_APPROVAL_REQUIRED !== undefined) {
return { statusCode: 409, errorCode: "SETTING_FORCED_BY_ENV", message: "Member approval setting is controlled by environment variable" };
}This would provide defense in depth and ensure the configuration can't be circumvented.
| } | ||
|
|
||
| const isDisabled = isLoading || forceMemberApprovalRequired !== undefined; | ||
| const showForceMessage = forceMemberApprovalRequired !== undefined; |
There was a problem hiding this comment.
similar to the forceEnableAnonymousAccess flag, we probably only want to show this message if forceMemberApprovalRequired is true
| memberApprovalRequired: env.FORCE_MEMBER_APPROVAL_REQUIRED === 'true' ? true : | ||
| env.FORCE_MEMBER_APPROVAL_REQUIRED === 'false' ? false : | ||
| true, // default to true if FORCE_MEMBER_APPROVAL_REQUIRED is not set |
There was a problem hiding this comment.
| memberApprovalRequired: env.FORCE_MEMBER_APPROVAL_REQUIRED === 'true' ? true : | |
| env.FORCE_MEMBER_APPROVAL_REQUIRED === 'false' ? false : | |
| true, // default to true if FORCE_MEMBER_APPROVAL_REQUIRED is not set | |
| ...(env.FORCE_MEMBER_APPROVAL_REQUIRED === 'true' && { | |
| memberApprovalRequired: true, | |
| }) |
default values for data in the db are set by the schema so its best to not have them set in the code. We only want to set this if the env var is true anyways
| } | ||
| } | ||
|
|
||
| const isDisabled = isLoading || forceMemberApprovalRequired !== undefined; |
There was a problem hiding this comment.
similar to forceEnableAnonymousAccess we only need to disable if the forceMemberApprovalRequired flag is true
|
|
||
| // Auth | ||
| FORCE_ENABLE_ANONYMOUS_ACCESS: booleanSchema.default('false'), | ||
| FORCE_MEMBER_APPROVAL_REQUIRED: booleanSchema.optional(), |
There was a problem hiding this comment.
following the rest of the changes to only force UI stuff when this is enabled, we can also default this to false to match the other env var
|
@msukkari, I think it is best for you to take this over or for me to close. I don't have the context you do and I think you are requesting changes to nearly every line. :) |
2 participants
FORCE_ENABLE_ANONYMOUS_ACCESS.Summary by CodeRabbit
New Features
Documentation
Chores